Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: editorial improvements, typo fixes #4712

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Dec 13, 2023

Signed-off-by: David Karlsson [email protected]

q4 freshness updates

carries followups:

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@dvdksn
Copy link
Contributor Author

dvdksn commented Dec 13, 2023

@thaJeztah let me see if I can carry a couple follow-ups from the previous refresh as well

@dvdksn dvdksn marked this pull request as draft December 13, 2023 15:07
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looking good; left some comments/suggestions, and recalled some "related" changes that were never finished.

docs/reference/commandline/attach.md Outdated Show resolved Hide resolved
docs/reference/commandline/attach.md Outdated Show resolved Hide resolved
docs/reference/commandline/attach.md Outdated Show resolved Hide resolved
Comment on lines +194 to +195
> TLS version 1.0 and higher is supported. Protocols SSLv3 and below are not
> supported for security reasons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me; we need to verify what TLS versions we support now (it's very likely we no longer support 1.0)

@@ -687,7 +685,7 @@ To set the DNS search domain for all Docker containers, use:
$ sudo dockerd --dns-search example.com
```

### Allow push of nondistributable artifacts
### Allow push of non-distributable artifacts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me; we need to check the status on this one. I know the "foreign layers" feature was deprecated by the OCI, and we may be enabling this by default now (or are planning to)

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
Comment on lines +178 to +183
- config (`config=<name or id>`)
- container (`container=<name or id>`)
- daemon (`daemon=<name or id>`)
- event (`event=<event action>`)
- image (`image=<repository or tag>`)
- label (`label=<key>` or `label=<key>=<value>`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminds me I had a PR to rewrite this to use tables, but some of the combinations in my examples were wrong, as combining some did not produce the same as --type 🤔 (IIRC); perhaps you're interested in carrying that PR though;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me carry this one, but I'll do it in a follow-up as not to block this one since there's an open technical question

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to merge the events and system_events pages. Currently information is spread between them (and they may be out of sync).

I recall doing that as part of my "WIP use canonical URLs" but that got bit-rot.

@dvdksn dvdksn marked this pull request as ready for review December 13, 2023 19:00
@dvdksn dvdksn changed the title docs: minor editorial improvements, typo fixes docs: editorial improvements, typo fixes Dec 13, 2023
@dvdksn
Copy link
Contributor Author

dvdksn commented Dec 18, 2023

@thaJeztah I pushed an additional update as a fixup (to make it easier to review), see
2b252a8

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM (thanks!)

found a couple of minor issues, and one or two that may need checking (sudo and env-var, and perhaps update that redirected URL)

`tar` UNIX format and can be compressed with any one of the 'xz', 'bzip2',
'gzip' or 'identity' (no compression) formats.
`tar` Unix format and can be compressed with any one of the `xz`, `bzip2`,
`gzip` or `identity` (no compression) formats.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think zstd may now also work, but we'd have to check if it does (so no need to change for this PR)

@@ -274,7 +272,7 @@ $ docker build - < context.tar.gz
```

This will build an image for a compressed context read from `STDIN`. Supported
formats are: bzip2, gzip and xz.
formats are: `bzip2`, `gzip` and `xz`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (also for follow-ups)

docs/reference/commandline/build.md Outdated Show resolved Hide resolved
docs/reference/commandline/config_rm.md Outdated Show resolved Hide resolved
docs/reference/commandline/context_create.md Outdated Show resolved Hide resolved
Comment on lines 865 to 866
$ export DOCKER_TMPDIR=/mnt/disk2/tmp
$ /usr/local/bin/dockerd --data-root /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1
$ sudo dockerd --data-root /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh; does this work? sudo may be omitting the DOCKER_TMPDIR env-var (either requires sudo -E, or we should inline the env-var instead of exporting.

Also wondering if we should leave the > /var/lib/docker-machine/docker.log 2>&1 as an exercise to the reader (managing detached processes can be fiddly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even know what the redirect is doing in this case so I am happy to drop it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

Merging #4712 (2b252a8) into master (103840e) will decrease coverage by 0.03%.
Report is 21 commits behind head on master.
The diff coverage is n/a.

❗ Current head 2b252a8 differs from pull request most recent head 6fd4cff. Consider uploading reports for the commit 6fd4cff to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4712      +/-   ##
==========================================
- Coverage   59.68%   59.65%   -0.03%     
==========================================
  Files         287      285       -2     
  Lines       24865    24757     -108     
==========================================
- Hits        14841    14770      -71     
+ Misses       9138     9100      -38     
- Partials      886      887       +1     

@thaJeztah thaJeztah added this to the 25.0.0 milestone Dec 19, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 5ba998d into docker:master Dec 19, 2023
76 checks passed
@dvdksn dvdksn deleted the docs-tier1-fresshness-q4 branch December 19, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants